ARROW-7979: [C++] Add experimental buffer compression to IPC write path. Add "field" selection to read path. Migrate some APIs to Result<T>. Read/write Message metadata by wesm · Pull Request #6638 · apache/arrow

wesm · 2020-03-17T01:16:00Z

I apologize for the complexity of this patch, there was some necessary refactoring and then as I needed to add a IpcReadOptions struct, it made sense to port some APIs from Status to Result.

Summary of what's here

Add IpcReadOptions struct
Rename IpcOptions to IpcWriteOptions
Serialize and write custom_metadata field in Message Flatbuffer struct
Move MemoryPool* arguments in IPC functions to options structs
Adds EXPERIMENTAL compression option to IpcOptions (which should be renamed IpcWriteOptions) which causes each buffer in a record batch body to be separately compressed with e.g. ZSTD. This is intended to be used internally in Feather V2 and not exported for general use. Based on the mailing list discussion, if we were to adopt this in the IPC protocol then this can be promoted to non-experimental
Add "included_fields" option to select a subset of fields to load from a record batch when doing an IPC load. The motivation for this was to avoid unnecessary decompression when reading a subset of columns from an IPC stream
Migrate most IPC read APIs to use Result, deprecate old methods
Deprecate Status-returning IPC write method

Some other small changes:

Add check_metadata option to RecordBatch::Equals
Add Codec::GetCompressionType method for looking up Compression::type given a codec name

I don't have size/perf benchmarks about how the compression helps with file sizes or read performance, so I'll do that next in the course of completing ARROW-5510 ("Feather V2" file format)

github-actions · 2020-03-17T01:16:32Z

https://issues.apache.org/jira/browse/ARROW-7979

wesm · 2020-03-17T03:06:39Z

I'll fix the glib issues and other things tomorrow

pitrou

I haven't read a lot of this, so just a few comments.

pitrou · 2020-03-18T10:57:24Z

There are at most 3 or 4 buffers in a Arrow array, and they have very different sizes. I don't think parallelizing at this level makes sense.

Right. The parallelization needs to happen at the field-level, which will require some refactoring. I'll open a follow up JIRA since it's out of scope for this PR

bkietz

This looks great! Lots of comments, mostly code style/nits

bkietz · 2020-03-18T17:28:57Z

nit: avoid using Result::Value unless wrapping a result returning API into a status returning API

Suggested change

auto status = ipc::RecordBatchFileReader::Open(std::move(input)).Value(&reader);

if (!status.ok()) {

return status.WithMessage("Could not open IPC input source '", source.path(),

"': ", status.message());

}

auto maybe_reader = ipc::RecordBatchFileReader::Open(std::move(input));

auto status = maybe_reader.status();

if (!status.ok()) {

return status.WithMessage("Could not open IPC input source '", source.path(),

"': ", status.message());

}

return std::move(maybe_reader).ValueOrDie();

Why is using Result::Value bad? Not obvious from reading the docstrings

It's not bad, just less preferred due to mixing argout Status return and Result usage

wesm · 2020-03-19T21:54:36Z

I think I've addressed almost all of the comments. I'll try to get all the builds passing (C++/Python/R/GLib all passing for me locally)

wesm · 2020-03-20T02:22:54Z

Hmm looks like I broke something related to Maps

################# FAILURES #################
FAILED TEST: map C++ producing,  C++ consuming

FAILED TEST: map C++ producing,  Java consuming

FAILED TEST: map C++ producing,  JS consuming

FAILED TEST: map Java producing,  C++ consuming

FAILED TEST: map JS producing,  C++ consuming

FAILED TEST: map C++ producing,  C++ consuming

FAILED TEST: map C++ producing,  Java consuming

FAILED TEST: map Java producing,  C++ consuming

I'll fix that plus the Windows issue tomorrow. If I could get the thumbs up/down on the big picture stuff on the meantime that would be great

bkietz

big picture: +1

wesm · 2020-03-20T18:04:38Z

@lidavidm some of the Flight integration tests failed with this error, do you know what it could be about?

  File "/arrow/dev/archery/archery/integration/tester_cpp.py", line 91, in flight_server
    raise RuntimeError(
RuntimeError: Flight-C++ server did not start properly, stdout:
/opt/conda/envs/arrow/lib/libarrow.so.100(+0x12faf48)[0x7f9280b1ef48]


stderr:
E0320 17:56:33.746761714   10694 socket_utils_common_posix.cc:222] check for SO_REUSEPORT: {"created":"@1584726993.746751914","description":"SO_REUSEPORT unavailable on compiling system","file":"../src/core/lib/iomgr/socket_utils_common_posix.cc","file_line":190}
E0320 17:56:33.746901914   10694 server_chttp2.cc:40]        {"created":"@1584726993.746866914","description":"No address added out of total 1 resolved","file":"../src/core/ext/transport/chttp2/server/chttp2_server.cc","file_line":395,"referenced_errors":[{"created":"@1584726993.746865414","description":"Failed to add any wildcard listeners","file":"../src/core/lib/iomgr/tcp_server_posix.cc","file_line":342,"referenced_errors":[{"created":"@1584726993.746848214","description":"Address family not supported by protocol","errno":97,"file":"../src/core/lib/iomgr/socket_utils_common_posix.cc","file_line":420,"os_error":"Address family not supported by protocol","syscall":"socket","target_address":"[::]:33625"},{"created":"@1584726993.746865114","description":"Unable to configure socket","fd":5,"file":"../src/core/lib/iomgr/tcp_server_utils_posix_common.cc","file_line":216,"referenced_errors":[{"created":"@1584726993.746862214","description":"Address already in use","errno":98,"file":"../src/core/lib/iomgr/tcp_server_utils_posix_common.cc","file_line":189,"os_error":"Address already in use","syscall":"bind"}]}]}]}
/arrow/cpp/src/arrow/flight/test_integration_server.cc:165:  Check failed: _s.ok() Operation failed: g_server->Init(options)
Bad status: Unknown error: Server did not start properly

lidavidm · 2020-03-20T18:16:03Z

The relevant bit is Address already in use so it seems the Flight tests are not safe to be run in parallel, even with the port allocation code in the test runner. We should eventually have the server bind on port 0 and have the integration test runner parse the port out of its output to avoid races like this.

lidavidm · 2020-03-20T18:17:07Z

There's https://issues.apache.org/jira/browse/ARROW-6528 which is about unit tests, I'll add a related JIRA about fixing this in integration too.

https://issues.apache.org/jira/browse/ARROW-8176

wesm · 2020-03-20T18:22:17Z

@lidavidm thanks, I'll set the integration test task to run serially for now

wesm · 2020-03-20T21:55:56Z

The UBSAN failure is a weird one. It's fixed by the Flatbuffers 1.12 upgrade ARROW-8178, so I'll get that merged once the builds run and then rebase this

kou

I want to review this in a few days.

wesm · 2020-03-22T21:06:33Z

@nealrichardson any advice about the R test failure? The output log didn't give any clues and all the tests pass for me locally on Linux

nealrichardson · 2020-03-23T15:14:38Z

Hmm. It looks like it just dies when loading the package to run the 32-bit version of the tests, but the 64-bit version runs fine.

wesm · 2020-03-23T23:12:40Z

Cross-referencing these results. The compression looks highly beneficial in terms of reduction in message size for modest decompression time

#6694 (comment)

I would guess that this would yield significant throughput benefits for Flight users. cc @lidavidm

I'll update the mailing list thread about this

kou

+1

We can merge this once the R problem is fixed.

kou · 2020-03-24T00:13:06Z

Should we add const IpcWriteOptions& options to GrpcStreamWriter::Open() instead?

Yes, we should do that. It will have to happen after @lidavidm's refactoring goes in. Opening a JIRA https://issues.apache.org/jira/browse/ARROW-8190

add empty reader_internal.h Some more temporary cutting away / simplification Ongoing refactoring More refactoring and cleaning of Feather V1 More cleaning, simplification Cleaning fixes Get feather.cc compiling again Slightly more working Feather tests passing again Some code reorganization, simply IPC writing a bit Start writer_internal.h Start writer_internal.h Incomplete refactoring Externalize enough of IPC writer machinery to be able to customize for Feather Remove overly elaborate externalization of IPC writer internals, implement compression as part of IpcOptions Work on serializing custom_metadata in Message Fix compilation Test empty metadata case also iwyu Some more work on compression complete more of decompression Decompression draft Implement various deprecations, and get compression tests passing Revert Feather changes

…on on IPC reads

Because they are provided in 0.16.0. Keeping them provides backward compatibility.

wesm · 2020-03-24T01:11:03Z

This still fails.

EDIT: What I wrote about STOP_OR_VALUE was not right.

I'm not sure why this is failing. I'm going to rebase but we probably need to get some debugging help or instructions how to run this build locally on Windows

wesm · 2020-03-24T01:53:31Z

I built the project in 32-bit mode with -m32 on Linux and the tests pass fine. Not sure how to proceed with debugging

nealrichardson · 2020-03-24T02:33:13Z

@wesm assuming you have a windows box with mingw stuff,

Go to https://cran.r-project.org/bin/windows/ and install R
Follow the Rtools link there and install Rtools 3.5
Build the C++ library with makepkg-mingw following https://github.com/apache/arrow/blob/master/ci/scripts/r_windows_build.sh
Set the file path of the resulting .zip file of the libs as the env var RWINLIB_LOCAL and then install the R package

See https://github.com/apache/arrow/blob/master/.github/workflows/r.yml#L157-L177

wesm · 2020-03-24T02:39:29Z

@nealrichardson OK I'll give it a try tomorrow. We are definitely weak when it comes to Windows issues, very few developers are equipped to debug these kinds of issues locally

wesm · 2020-03-24T02:44:26Z

@nealrichardson I don't have the "mingw stuff" (Python uses MSVC to build things), what other machine setup do I have to do outside of installing RTools?

nealrichardson · 2020-03-24T03:03:39Z

I believe you need to install https://www.msys2.org/. The R Windows GHA uses https://github.com/numworks/setup-msys2 for that; Kou added some scripts ci/scripts/msys* that the Ruby jobs use.

This reverts commit db5a9a8.

wesm · 2020-03-24T19:08:03Z

I'm working on this, but I have almost no idea what I'm doing. For example, there are so many different shells (normal Windows shell, Powershell, msys2 shell), how do I get the commands listed at

https://github.com/apache/arrow/blob/master/.github/workflows/cpp.yml#L278

to work? Flying blind

wesm · 2020-03-24T19:12:47Z

It seems I'm supposed to be using Powershell. It would be nice at some point to have easy-to-follow source build instructions for R on Windows so the next person who runs into a problem doesn't have to dig through all these CI scripts to figure out what is what

wesm · 2020-03-24T19:26:47Z

I'm giving up, can't figure out how to configure my environment to be able to run https://github.com/apache/arrow/blob/master/ci/scripts/r_windows_build.sh after about an hour of trying. I will need help from someone else to find out why the 32-bit R build is broken

nealrichardson · 2020-03-24T22:36:59Z

@wesm I've reproduced the 32-bit windows R segfault and pushed a skip for the test so that this is unblocked. The code that triggers the segfault is in the IPC format Datasets code, presumably in the scan operation because it succeeds in instantiating the dataset. @bkietz maybe you can take a look too and see if you see something suspect there?

nealrichardson · 2020-03-24T22:48:54Z

Before I forget (high mental load average right now), here's how I reproduced it:

Forget msys2 and all that. I used ursa-labs/arrow-r-nightly to build the C++ library for Windows on this branch and publish the artifact to bintray with an arbitrary version number (0.16.0.666666): https://github.com/ursa-labs/arrow-r-nightly/runs/531644004
In my local Windows 10 VM with R and Rtools installed, (a) set the arbitrary version number in r/DESCRIPTION to match the thing I put on bintray, and (b) remotes::install_local("Z:\\arrow\\r") to install dependencies and then the package itself, compiling the bindings from source.
Since this only crashed on 32-bit R, run that from the Command Prompt. C:\Program Files\R\R-3.6.0\bin\i386\R.exe
Since running R with gdb on Windows sounds terrible, I instead ran the tests with more verbosity, using the "location" test reporter, so that I could see where exactly it died:

setwd("Z:\\arrow\\r\\tests/")
library("testthat")
test_check("arrow", reporter="location")

That gave a bunch of output, until it crashed and I was dumped back to the command prompt:

...
Start test: Simple interface for datasets (custom ParquetFileFormat)
  test-dataset.R#124:1 [success]
End test: Simple interface for datasets (custom ParquetFileFormat)

Start test: Hive partitioning
  test-dataset.R#129:1 [success]
  test-dataset.R#130:1 [success]
End test: Hive partitioning

Start test: Partitioning inference
  test-dataset.R#144:1 [success]
  test-dataset.R#145:1 [success]
  test-dataset.R#158:1 [success]
  test-dataset.R#159:1 [success]
End test: Partitioning inference

Start test: IPC/Arrow format data
  test-dataset.R#172:1 [success]
  test-dataset.R#173:1 [success]

C:\Program Files>

So the error was something happening after L173 in test-dataset.R.

kou · 2020-03-25T00:17:42Z

"Dev / Source Release and Merge Script" job failure will be fixed by #6711.

wesm · 2020-03-25T02:02:18Z

Thank you @nealrichardson, that's definitely helpful information.

I looked at file_ipc.cc and didn't see anything especially concerning. I don't know if it's related but there are various usages of Result::ValueOrDie in the datasets implementation, we could turn on logging to see of one of those "shouldn't fail" conditions is occurring somehow

wesm · 2020-03-25T02:05:09Z

One change that this PR makes is making RecordBatchFileReader an abstract class with pure virtual methods. I don't see why that would cause a problem but...

nealrichardson · 2020-03-25T02:33:15Z

Beats me. I'll leave it to you to decide whether to merge this and create a followup for that or whether you want to hold this to debug further. If it were me, I'd do the former: IPC-format Datasets aren't a critical feature. And I suspect you don't feel like debugging 32-bit windows any further either ;)

wesm · 2020-03-25T15:40:12Z

OK, I'll merge this. Is there someone who can help us get a backtrace with gdb so we can try to find out what is happening?

wesm force-pushed the ARROW-7979 branch from 8443555 to a711e56 Compare March 17, 2020 01:48

pitrou reviewed Mar 18, 2020

View reviewed changes

bkietz requested changes Mar 19, 2020

View reviewed changes

wesm force-pushed the ARROW-7979 branch 2 times, most recently from 11fa5ad to 2d17d32 Compare March 19, 2020 21:53

bkietz approved these changes Mar 20, 2020

View reviewed changes

wesm force-pushed the ARROW-7979 branch from d274741 to dc29ca2 Compare March 20, 2020 18:34

wesm mentioned this pull request Mar 20, 2020

ARROW-8178: [C++] Update to Flatbuffers 1.12.0 #6678

Closed

wesm force-pushed the ARROW-7979 branch from dc29ca2 to aab071e Compare March 20, 2020 23:01

kou reviewed Mar 22, 2020

View reviewed changes

Comment thread cpp/src/arrow/ipc/writer.h Outdated

kou approved these changes Mar 24, 2020

View reviewed changes

wesm added 7 commits March 23, 2020 20:04

Rebase

a18f105

Fix benchmark

3bc293d

Proposed API for field projections on reading record batch

d51ff24

Rename confusing IPC function

ff3cbde

Use Result<T> for reader APIs, implement unit tests for field selecti…

7c98ee8

…on on IPC reads

Switch to result APIs in Python

c4d5454

wesm and others added 5 commits March 23, 2020 20:04

Add missing export

23f27d6

IWYU

3e8a341

Fix bugs turned up by CI, fuzzing corpus

67ad393

Restore Result-returning RecordBatch{Stream,File}Writer::Open

bffef74

Because they are provided in 0.16.0. Keeping them provides backward compatibility.

Use garrow::check() with Result

b9f47c5

wesm force-pushed the ARROW-7979 branch from fda25eb to b9f47c5 Compare March 24, 2020 01:14

wesm added 2 commits March 23, 2020 21:37

Revert R changes temporarily

db5a9a8

Restore deprecated Result-returning functions

e20343e

Revert "Revert R changes temporarily"

ce08470

This reverts commit db5a9a8.

Skip dataset test that causes segfault on 32-bit Windows R

e7a4943

wesm closed this in fcde1eb Mar 25, 2020

wesm deleted the ARROW-7979 branch March 25, 2020 23:08

This was referenced Apr 6, 2020

[C++] Implement experimental buffer compression in IPC messages #24197

Closed

[R][C++] Fix crashing test in test-dataset.R on 32-bit Windows from ARROW-7979 #24414

Closed

-  auto status = ipc::RecordBatchFileReader::Open(std::move(input)).Value(&reader);
-  if (!status.ok()) {
-    return status.WithMessage("Could not open IPC input source '", source.path(),
-                              "': ", status.message());
-  }
+  auto maybe_reader = ipc::RecordBatchFileReader::Open(std::move(input));
+  auto status = maybe_reader.status();
+  if (!status.ok()) {
+    return status.WithMessage("Could not open IPC input source '", source.path(),
+                              "': ", status.message());
+  }
+  return std::move(maybe_reader).ValueOrDie();

Conversation

wesm commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 17, 2020

Uh oh!

wesm commented Mar 17, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

wesm Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

bkietz Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

wesm Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

bkietz Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wesm commented Mar 19, 2020

Uh oh!

wesm commented Mar 20, 2020

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Mar 20, 2020

Uh oh!

lidavidm commented Mar 20, 2020

Uh oh!

lidavidm commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Mar 20, 2020

Uh oh!

wesm commented Mar 20, 2020

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wesm commented Mar 22, 2020

Uh oh!

nealrichardson commented Mar 23, 2020

Uh oh!

wesm commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

wesm Mar 24, 2020

wesm commented Mar 17, 2020 •

edited

Loading

lidavidm commented Mar 20, 2020 •

edited

Loading

wesm commented Mar 23, 2020 •

edited

Loading

wesm commented Mar 24, 2020 •

edited

Loading

nealrichardson commented Mar 24, 2020 •

edited

Loading